chore: continue with miden-base update (3154a3)#1815
chore: continue with miden-base update (3154a3)#1815juan518munoz merged 7 commits intoandrew-migrate-to-next-basefrom
3154a3)#1815Conversation
a58d2f5 to
bd0859a
Compare
0d25a9a to
6c10836
Compare
| /// Represents a `proto::rpc::SyncStateResponse` with fields converted into domain types. | ||
| /// Represents the composed result of `sync_notes`, `sync_chain_mmr`, and `sync_transactions` | ||
| /// with fields converted into domain types. | ||
| pub struct StateSyncInfo { |
There was a problem hiding this comment.
Maybe we should move this elsewhere as it's no longer part of the domain
There was a problem hiding this comment.
Agree, maybe into crates/rust-client/src/sync/state_sync.rs since it's only used there.
There was a problem hiding this comment.
I think we should keep this at the rpc module level (ie move it up one more mod)
| # inputs are passed as foreign_procedure_inputs: | ||
| # [slot_id_prefix, slot_id_suffix, KEY, pad(10)] | ||
| exec.active_account::get_map_item |
There was a problem hiding this comment.
Should we still push the storage slot here? Why was this changed to assumed inputs?
There was a problem hiding this comment.
I think both approaches test effectively the same, so no need to change it necessarily, but yeah not sure why this was generalized (now you can query any slot)
|
|
||
| ### Changes | ||
|
|
||
| * [BREAKING] Incremented MSRV to 1.91. |
There was a problem hiding this comment.
Lets add the API changes for the rpc trait
| struct StatusService; | ||
|
|
||
| #[async_trait::async_trait] | ||
| impl worker_status_api_server::WorkerStatusApi for StatusService { | ||
| async fn status( | ||
| &self, | ||
| _: tonic::Request<()>, | ||
| ) -> Result<tonic::Response<proto::WorkerStatus>, tonic::Status> { | ||
| Ok(tonic::Response::new(proto::WorkerStatus { | ||
| version: env!("CARGO_PKG_VERSION").to_string(), | ||
| supported_proof_type: proto::ProofType::Transaction as i32, | ||
| })) | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need to implement this? Maybe we can remove the status service if we don't use it.
| pub fn with_supports_all_types(self) -> Self { | ||
| let metadata = self.0.metadata().clone().with_supports_all_types(); | ||
| let code = self.0.component_code().clone(); | ||
| let slots = self.0.storage_slots().to_vec(); | ||
| AccountComponent( | ||
| NativeAccountComponent::new(code, slots, metadata) | ||
| .expect("reconstructing component with updated metadata should not fail"), | ||
| ) |
There was a problem hiding this comment.
This method was removed from the AccountComponent struct so maybe we should remove it here as well, instead of working around it.
| /// highest `block_num`. This replicates the old `SyncState` behavior where the node returned | ||
| /// the latest account commitment per account in the synced range. | ||
| fn derive_account_commitment_updates( |
There was a problem hiding this comment.
Maybe rename this to something like derive_latest_commitments()? Also, I'd rather not reference the "old" implementation in the comments.
3154a3)
igamigo
left a comment
There was a problem hiding this comment.
LGTM! Good work. We can address the remaining comments once we merge this to the base branch
| # inputs are passed as foreign_procedure_inputs: | ||
| # [slot_id_prefix, slot_id_suffix, KEY, pad(10)] | ||
| exec.active_account::get_map_item |
There was a problem hiding this comment.
I think both approaches test effectively the same, so no need to change it necessarily, but yeah not sure why this was generalized (now you can query any slot)
| /// Represents a `proto::rpc::SyncStateResponse` with fields converted into domain types. | ||
| /// Represents the composed result of `sync_notes`, `sync_chain_mmr`, and `sync_transactions` | ||
| /// with fields converted into domain types. | ||
| pub struct StateSyncInfo { |
There was a problem hiding this comment.
I think we should keep this at the rpc module level (ie move it up one more mod)
| // Fallback for transactions with unauthenticated input notes: the node | ||
| // authenticates these notes during processing, which changes the transaction | ||
| // ID. Match by account ID and pre-transaction state instead. | ||
| if let Some(transaction) = self.transactions.values_mut().find(|tx| { | ||
| tx.details.account_id == transaction_inclusion.account_id | ||
| && tx.details.init_account_state == transaction_inclusion.initial_state_commitment | ||
| }) { | ||
| transaction.commit_transaction(transaction_inclusion.block_num, timestamp); |
There was a problem hiding this comment.
Not entirely sure we would need this, but I'll need to investigate more
| pub fn with_supports_all_types(self) -> Self { | ||
| let metadata = self.0.metadata().clone().with_supports_all_types(); | ||
| let code = self.0.component_code().clone(); | ||
| let slots = self.0.storage_slots().to_vec(); | ||
| AccountComponent( | ||
| NativeAccountComponent::new(code, slots, metadata) | ||
| .expect("reconstructing component with updated metadata should not fail"), | ||
| ) |
Continue with node & base update to update the
miden-basedependency to commit0904e2c61.Note:
or expose the previously used structs again.